Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[zombinet] initial implementation of zombienet backchannel #4377

Merged
merged 26 commits into from
Jan 7, 2022
Merged

[zombinet] initial implementation of zombienet backchannel #4377

merged 26 commits into from
Jan 7, 2022

Conversation

pepoviola
Copy link
Contributor

@pepoviola pepoviola commented Nov 25, 2021 •
edited
Loading

Hi @drahnr / @Lldenaurois,
This is the initial implementation of the backchannel for zombienet, targeting #4272.

The general idea is that there is a server side running in zombienet environment that has an http and a ws listeners,

  • The http interface accept post request to store new items that should be available for use by the test-runner.
  • The ws interface broadcasted those new items to all connected malus actors.

Notes:

  • The two env vars are defaulted to the ones we need to use in our CI, since we can resolve backchannel to the correct endpoint.
  • The connect fn create the ws connection and return the receiver part in the inner field.
  • I only tested locally (with a small client that I create), so we need to complete the integration to the malus crate in order to test the complete flow locally or in the CI.
  • I'm not sure if the use of the scale encode if correct because in the zombienet side is stored as string.
  • A simple test mocking the zombienet server should be implemented after complete the integration.

Thanks!

@pepoviola pepoviola added I5-tests Tests need fixing, improving or augmenting. A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes labels Nov 25, 2021
@pepoviola pepoviola added the C1-low PR touches the given topic and has a low impact on builders. label Nov 25, 2021
drahnr
drahnr suggested changes Nov 26, 2021
View reviewed changes
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline comments

node/zombienet-backchannel/src/lib.rs Outdated Show resolved Hide resolved
node/zombienet-backchannel/src/lib.rs Outdated Show resolved Hide resolved
node/zombienet-backchannel/src/lib.rs Outdated Show resolved Hide resolved
node/zombienet-backchannel/src/lib.rs Outdated Show resolved Hide resolved
node/zombienet-backchannel/src/lib.rs Outdated Show resolved Hide resolved
node/zombienet-backchannel/Cargo.toml Outdated Show resolved Hide resolved
@pepoviola
Copy link
Contributor Author

Hi @drahnr, I updated the implementation follow the pattern from our last talk. Now you have an init fn that connect you to the ws server on zombienet side and then two fn

  • subscribe : give you a receiver to get the new item from the ws
  • send : Send new items to the ws.

Let me know if you want to change/refactor something.

Examples of usage could be:

  • receive part
    let backchannel = ZombienetBackchannel::init().await.expect("Error connecting ws");

    let mut rx = backchannel.subscribe();
    while let Ok(backchannel_item) = rx.recv().await {
        println!("{:?}", backchannel_item);
    }
  • send
     let backchannel = ZombienetBackchannel::init().await.expect("Error connecting ws");
     backchannel.send(<key>, <value>)

Ping @Lldenaurois, let me know if you want to chat about test cases 0002/0003 for malus integration.

Thanks!!

@pepoviola pepoviola requested a review from drahnr December 7, 2021 17:52
drahnr
drahnr approved these changes Dec 14, 2021
View reviewed changes
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM besides the lack of documentation :)

@pepoviola
Copy link
Contributor Author

Thanks for the feedback @drahnr, I added some docs to those methods :)
Thx!

@drahnr drahnr mentioned this pull request Dec 20, 2021
Closed
@drahnr
Copy link
Contributor

drahnr commented Jan 6, 2022

Could you merge master, so we can merge this :)

@pepoviola
Copy link
Contributor Author

Hi @drahnr, merged :) but now the smoke test is failing for the same reason of #4519 (comment) (the collator version).
Thanks!

@drahnr
Copy link
Contributor

drahnr commented Jan 7, 2022

Please merge master again

@pepoviola
Copy link
Contributor Author

Please merge master again

Done 👍, thanks!

@pepoviola
Copy link
Contributor Author

Hi @drahnr, thanks for the feedback. I already made those changes 👍
Thanks!

@drahnr drahnr merged commit bf91476 into master Jan 7, 2022
@drahnr drahnr deleted the javier-zombienet-backchannel branch January 7, 2022 16:48
Wizdave97 pushed a commit to ComposableFi/polkadot that referenced this pull request Feb 3, 2022
@pepoviola @Wizdave97
[zombinet] initial implementation of zombienet backchannel (paritytec…
7af87dc 
…h#4377)
@librelois librelois mentioned this pull request Feb 7, 2022
Closed
@drahnr drahnr mentioned this pull request Mar 14, 2022
Closed
9 tasks
@drahnr drahnr mentioned this pull request Apr 13, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@drahnr drahnr drahnr approved these changes

@Lldenaurois Lldenaurois Awaiting requested review from Lldenaurois

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. I5-tests Tests need fixing, improving or augmenting.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pepoviola @drahnr